Skip to content

Conversation

benrr101
Copy link
Contributor

Description

This is a fairly major change.

https://learn.microsoft.com/en-us/dotnet/framework/performance/constrained-execution-regions#code-not-permitted-in-cers

For the longest time, and especially as part of this common project merge, we have maintained these Constrained Execution Regions as part of our netfx implementation. However, they've been a major pain to work around, since they involve try/catch/finally structures that aren't needed in netcore, or catching exceptions that are never expected to be caught in netcore. There have been several approaches brought forward to conditionally include the CER structures - some better than others. While working on the SqlCommand merge, I suggested another approach that moves the CER structure to a helper (see #3514). This uncovered a major limitation on CER blocks - that function pointers/delegates cannot be used (see link above).

This prompted a bit more research, and it appears that our codebase is riddled with code that cannot be called from a CER block. Since .NET 2.0, CER has been a guideline instead of a hard rule, and CER execution has become best-effort rather than guaranteed, and as a result much more permissive in what can be prepared/executed. Even with obvious issues in the code, CI has passed for ages, and no users have complained of faults arising from CER failures. As such, the question arises - do we even need CER anymore? The answer to this, so far, seems to be no.

So, in this PR, I searched the codebase for instances of PrepareConstrainedRegion and just removed the entire try/catch/finally structure where possible.

Here's some examples of places where we're doing unapproved things inside CER blocks:

  • SqlCommand.InternalExecuteNonQuery
    • Called from SqlCommand.ExecuteNonQuery
    • Calls RunExecuteNonQueryTds which contains a lambda being called on timeout (among others)
  • SqlCommand.ExecuteReader
    • Explicitly allocates the reader inside the constrained execution region
    • Uses lambdas all over
  • SqlConnection.InternalOpenAsync
    • Called from SqlConnection.OpenAsync
    • Uses function pointer in retry callback
  • TdsParserStateObject.ReadSniError
    • Calls TdsParserStateObject.ProcessSniPacket which explicitly allocates a byte array

This PR is best viewed with whitespace changes ignored

Issues

Will make remaining work for #1261 easier

Testing

Project still builds, CI will hopefully be sufficient for validation of behavior.

@benrr101 benrr101 added this to the 7.0-preview1 milestone Jul 31, 2025
@benrr101 benrr101 requested review from saurabh500 and Copilot July 31, 2025 22:52
@benrr101 benrr101 added the Code Health 💊 Issues/PRs that are targeted to source code quality improvements. label Jul 31, 2025
@benrr101 benrr101 requested a review from a team as a code owner July 31, 2025 22:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes Constrained Execution Regions (CER) from the codebase, which have become obsolete and problematic to maintain. CER was originally designed to provide guarantees about code execution in the face of asynchronous exceptions, but has evolved into a best-effort mechanism that is no longer necessary for the project's goals.

Key changes include:

  • Removal of RuntimeHelpers.PrepareConstrainedRegions() calls and associated try/catch/finally blocks
  • Elimination of specific exception handling for OutOfMemoryException, StackOverflowException, and ThreadAbortException within CER blocks
  • Removal of CER-related attributes like [ReliabilityContract]

Reviewed Changes

Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
TdsParserStateObject.cs Removed CER preparation and ReliabilityContract attribute from IncrementPendingCallbacks method
TdsParserSessionPool.cs Removed CER-related comment about ThreadAbort handling
TdsParserSafeHandles.Windows.cs Removed CER preparation from SNIHandle constructor
SqlUtil.cs Removed CER blocks and specific exception handling from task continuation methods
SqlTransaction.cs Removed CER preparation and specific exception handling from transaction operations
SqlInternalConnection.cs Removed CER blocks and BestEffortCleanup methods
SqlDependencyListener.cs Removed CER preparation from impersonation context
SqlDependency.cs Removed CER preparation from Start/Stop methods
SqlDelegatedTransaction.cs Removed CER blocks from transaction operations
SqlDataReader.cs Removed extensive CER blocks from data reading operations
SqlCommandBuilder.cs Removed CER preparation from DeriveParameters method
SqlBulkCopy.cs Removed CER blocks from bulk copy operations
LocalDbApi.Windows.cs Removed CER preparation from LocalDB API calls
SqlClientMetrics.cs Removed ReliabilityContract attribute
WaitHandleDbConnectionPool.cs Removed CER blocks from connection pool operations
DbConnectionPoolAuthenticationContext.cs Removed ReliabilityContract attribute
SqlDataSourceEnumeratorNativeHelper.cs Removed CER preparation from data source enumeration
DbConnectionInternal.cs Removed ReliabilityContract attribute from DoomThisConnection
AdapterUtil.cs Removed ReliabilityContract attribute
SniNativeWrapper.cs Removed CER preparation from packet handling
TdsParserStateObjectNative.cs (netfx/netcore) Removed CER blocks from packet disposal
TdsParserStateObject.netfx/netcore.cs Removed CER preparation and RuntimeHelpers stub
TdsParser.netfx/netcore.cs Removed BestEffortCleanup methods and CER blocks
SqlInternalConnectionTds.cs (netfx/netcore) Removed CER blocks from connection operations
SqlConnectionHelper.cs Removed ReliabilityContract attribute
SqlConnection.cs (netfx/netcore) Removed CER blocks from connection operations
SqlCommand.cs (netfx/netcore) Removed extensive CER blocks from command execution
Comments suppressed due to low confidence (4)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSafeHandles.Windows.cs:204

  • Extra closing brace that should be removed. The try/finally block was removed but this closing brace was left behind.
        }

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:2767

  • Missing opening brace for the method. The CER block removal appears to have left the method definition incomplete.
        {

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs:192

  • Extra closing brace remaining after CER block removal. This should be removed to maintain proper brace matching.
        }

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs:459

  • Extra closing brace remaining after CER block removal. This should be removed to maintain proper brace matching.
        }

@benrr101
Copy link
Contributor Author

benrr101 commented Aug 4, 2025

@edwardneal @Wraith2 @ErikEJ Would love to get some feedback on this idea. It's definitely a Change ™️

@edwardneal
Copy link
Contributor

I definitely agree that the CERs shouldn't stay as they are; we should either write code which meets the reliability contract (and include the use case in CI, review with the SQL Server behaviour in mind, etc.) or stop advertising it as a capability. There's additional linked documentation which highlights other areas where we might also fall short.

This could have an impact on situations where someone is running SqlClient within a SQLCLR environment, but not using the context connection (i.e. a custom CLR stored procedure which instantiates a SqlConnection to a separate SQL Server instance.) I wasn't able to load the netfx SqlClient assembly when I tested in #2838, and at the time it wasn't a supported scenario.

I'm in favour of removing the CERs.

Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some removed catch blocks were calling methods that are no longer called. Should they be? Can those methods be removed now?

Copy link
Contributor

@mdaigle mdaigle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we haven't seen much demand for this because most customers who use a driver in SQLCLR are sticking with System.Data.SqlClient. SDS is a much more compelling option for SQLCLR because it avoids an extra package install. So I wouldn't necessarily take a lack of complaint to mean a lack of usage.

That said, the legacy SQLCLR code is currently holding us back in MDS and is broken. I'm not sure that keeping it in place significantly reduces the complexity of adding true support in the future. We can refer to this PR in the future as a starting point if we do decide to work on supporting it.

Found the following:
image

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 18, 2025

This is a topic i've discussed with David Engel in the past. My understanding is that M.D.SqlClient is not supported or usable within sql server. The only supported version is S.D.SqlClient. With the deprecation of S.D.SqlClient I'm not sure what this means for the SQLCLR feature but until there is official confirmation that M.D.SqlClient is supported in SQL Server I would not expect it to work and if someone tries it and it does work it may just be luck, not something to rely on.

Netcore and now NET is not and will never be a supported host in sql server because much of the work done to improve netcore over netfx was removal of things that SQLCLR needed. Things like being able to terminate at any arbitrary point in execution without corrupting the runtime are simply not possible and were a constant source of bugs. ThreadAbortException, CER and related work were part of this featureset and have been removed.

@benrr101 benrr101 requested a review from David-Engel August 19, 2025 20:57
paulmedynski
paulmedynski previously approved these changes Aug 22, 2025
Copy link

codecov bot commented Aug 22, 2025

Codecov Report

❌ Patch coverage is 64.29980% with 181 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.20%. Comparing base (55cc333) to head (3f29a69).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
...ient/src/Microsoft/Data/SqlClient/SqlDataReader.cs 64.23% 54 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 29 Missing ⚠️
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs 75.43% 28 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 27 Missing ⚠️
...icrosoft/Data/SqlClient/SqlDelegatedTransaction.cs 63.51% 27 Missing ⚠️
...Client/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs 0.00% 6 Missing ⚠️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 76.92% 3 Missing ⚠️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 77.77% 2 Missing ⚠️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 77.77% 2 Missing ⚠️
...netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs 50.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3535       +/-   ##
===========================================
+ Coverage   59.44%   70.20%   +10.76%     
===========================================
  Files         268      274        +6     
  Lines       62160    61403      -757     
===========================================
+ Hits        36950    43111     +6161     
+ Misses      25210    18292     -6918     
Flag Coverage Δ
addons 90.82% <ø> (?)
netcore 72.92% <62.02%> (+10.71%) ⬆️
netfx 70.01% <66.81%> (+6.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mdaigle
mdaigle previously approved these changes Aug 22, 2025
@mdaigle mdaigle self-assigned this Aug 26, 2025
@paulmedynski paulmedynski self-assigned this Aug 27, 2025
@paulmedynski paulmedynski merged commit 9041a52 into main Aug 28, 2025
250 checks passed
@paulmedynski paulmedynski deleted the dev/russellben/remove-cer branch August 28, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Health 💊 Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants